Add adversarial design and review for supplementary-file support, reconcile into design.md - #432
Conversation
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1621a42e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| - site rules: top-level entries MUST NOT resolve under `skills/`; per-skill entries MUST NOT be `SKILL.md` and MUST resolve below their skill's directory; | ||
| - collision-free across the whole planned entry set, where collision includes: exact spelling, canonical Unicode form (NFC/NFD aliases), portable case folding (case-insensitive filesystems), resolved source identity, and file/directory prefix conflicts (`foo` as a file vs. `foo/bar`). | ||
|
|
||
| Missing declared files fail the build with structured errors, and **all source inputs SHALL be validated before any `dist/` cleanup runs** — a declared input must never be destroyed before its missing-file error can be reported. Each failure class above maps to a distinct structured `ValidationError`, and the test suite SHALL carry a matrix with at least one case per class (traversal, absolute/drive paths, backslashes, NUL, empty/`.` segments, Unicode/case aliases, prefix collisions, symlinks, hard links, duplicates, exact-root-`facet.json` collisions, conventional-primary-path collisions, undeclared entries, missing declarations, tampered bytes). |
There was a problem hiding this comment.
A valid top-level declaration such as files: ["dist/README.md"] is permitted by D1/D7, but the existing build contract purges dist/ before writing output. This decision only moves cleanup after validation, so that file can validate successfully and then be deleted before collection/archive assembly, causing the build to fail (or omit it). Either reserve/exclude the output subtree from supplementary declarations or require inputs to be read/staged before dist/ is cleaned.
Useful? React with 👍 / 👎.
|
|
||
| ### D3: One shared archive-plan derivation | ||
|
|
||
| The protocol package SHALL expose a single pure operation that validates both declaration sites (per D7's grammar) and derives a **tagged archive plan**: every planned entry is classified as exactly one of `manifest`, `primary-asset`, `skill-companion` (carrying its owning skill), or `archive-only`. Build collection, per-entry hashing, archive verification, the parsed archive result, and installation MUST all consume this one operation. No stage maintains its own membership or classification logic. |
There was a problem hiding this comment.
D3 requires the shared protocol operation to be pure, yet D7 explicitly places existing-parent resolution, regular-file checks, symlink/hard-link rejection, and resolved-source-identity checks inside that operation. Those checks require filesystem access and do not exist for archive verification, so an implementation must either violate the protocol package's pure boundary or duplicate/omit validation. Define a pure manifest-to-path plan and a separate build-side source validation step (or an explicit injected source-facts input) so both callers can share the membership logic without this contradiction.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d1621a4. Configure here.
|
|
||
| This replaces the earlier optional-parameter shape (`companions?` beside `assetType`), which represented illegal combinations (an agent with companions; a skill call silently omitting its bundle) and policed them only by prose. Tagged variants give implementations an exhaustive branch. | ||
|
|
||
| A skill install SHALL be **one adapter operation with all-or-nothing semantics**: stage the complete replacement bundle, remove previously-owned companion paths absent from the new bundle, and commit — or roll back leaving no partial bundle. Skill deletion likewise removes the primary file plus all recorded owned companions as one operation, never touching unowned files. Expected failures are structured result values. |
There was a problem hiding this comment.
Incomplete skill adapter payloads
High Severity
The design for companion file ownership and deletion has some gaps. The D8 adapter delete contract doesn't provide prior companion paths needed for safe removal. Additionally, D10's receipt uses inner-archive paths while D8's skill payloads use skill-relative paths, and the path translation for offline deletion is undefined.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d1621a4. Configure here.
| - One shared derivation of the archive-entry set, consumed by build and verification alike — no duplicated allowlist logic. | ||
| - Preserve deterministic output within each archive format. Every build produced after the format transition MUST emit canonical `facetVersion: 0.2` output; consumers MUST continue accepting valid legacy `0.1` archives during the compatibility window. | ||
| - Make lockfile `0.2` the adapter-agnostic source of truth for every materialized logical file and its canonical per-file integrity, while the machine-local receipt records what this machine owns for rollback and offline removal. | ||
| - Make illegal states unrepresentable at the security- and data-loss-critical boundaries (adapter operations, receipt records, parsed archive results) via tagged unions, not optional fields plus prose invariants. |
There was a problem hiding this comment.
Receipt tagging goal unmet
Medium Severity
The D10 receipt design, by mirroring the lockfile's uniform files array, doesn't use tagged unions to prevent illegal states. This could allow invalid configurations, like an agent with companion files, which goes against the stated goal of making such states unrepresentable.
Reviewed by Cursor Bugbot for commit d1621a4. Configure here.
|
|
||
| Drift checking SHALL operate per locked file. Verbatim companion files are hashed directly from disk. For primary files whose adapter representation differs from archive bytes, the adapter `readAsset` contract SHALL return canonical logical content so the engine can compare the corresponding locked canonical integrity without encoding adapter-specific bytes in `facets.lock`. Reports SHALL identify the exact locked path that drifted. | ||
|
|
||
| The machine-local receipt SHALL mirror the successfully committed lockfile asset/file ownership set so offline removal and rollback remain exact even after a pulled lockfile drops an entry. The receipt remains adapter-agnostic and stores no adapter-encoded hashes. Receipt and lockfile changes SHALL commit in the same install transaction as materialization; rollback restores all three. Receipts remain untrusted input: identity, path containment, and file-integrity record validation MUST precede deletion, and unowned paths MUST never be deleted. The receipt schema version SHALL become `0.2`; legacy receipt version `1` MAY be refined to primary-only file sets because the legacy system could not install companions. |
There was a problem hiding this comment.
Ownership path model conflict
High Severity
D10 stores owned paths as canonical inner-archive paths in the lockfile and mirrored receipt, while D8 companion maps and containment checks use skill-root-relative paths. No translation rule is specified, and companion drift is required to hash bytes directly from disk even though adapters alone own on-disk layout. Offline removal, stale-companion cleanup, and drift checks cannot agree on which path grammar to use.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d1621a4. Configure here.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "authoredAt": "2026-07-13T18:10:26Z", | ||
| "comparedAt": "2026-07-13T18:19:40Z", | ||
| "reconciledAt": "2026-07-13T18:41:00Z", | ||
| "notes": "Adopted all 10 review findings (nothing material rejected; Main's structural base retained per the review itself). Added D3 shared tagged archive-plan derivation as single source of membership/classification; 0.2 build manifest replaces parallel assets+files maps with one all-entry hash map + strict version dispatch; protocol package and adapter SDK each marked as new major releases; D7 grammar expanded with exhaustive path/tar checks (NUL, drive prefixes, Unicode/case-fold aliases, prefix collisions, symlink/hardlink identity, duplicate headers, non-regular entries, dist/ cleanup ordering) + per-class test matrix; D8 adapter contract rewritten to tagged payload unions with atomic stage/commit/rollback skill bundles; D9 receipt rewritten to tagged records with complete owned path sets, legacy tuples refined to empty companion sets (removed the inconsistent one-install-cycle orphan claim), receipt+materialization as one rollback unit; D10 scaffold writes README undeclared during rollout window and edit gains scaffold-or-remove for vanished declarations; Migration Plan reordered consumer-first with cafe as hard gate and irreversibility note; old-builder-ignores-files hazard added to Risks; all three Open Questions closed as decisions." |
There was a problem hiding this comment.
The notes field states the design adopted "protocol package and adapter SDK each marked as new major releases," but design.md D4 explicitly chose the opposite: "breaking contract changes SHALL increment each package's minor version rather than its major version" and the alternatives section lists "package major releases (rejected: project policy uses minor releases for breaking changes while packages remain pre-1.0)."
This inaccuracy means the adversarial review trail records a finding as "adopted" when it was in fact explicitly rejected. A future reader consulting this state file to understand the design's reasoning — or to audit which blocking items were resolved — will see a contradictory picture and may reopen a debate the design already settled.
| ### D4: Build manifest — unconditional 0.2 output; strict version dispatch; pre-1.0 minor releases | ||
|
|
||
| Every build produced after this change SHALL emit `facetVersion: 0.2`, whether or not the facet declares supplementary files. Asset-only facets therefore use the same current format as facets with supplementary files. Producers SHALL NOT conditionally emit `0.1`; `0.1` remains a legacy input format supported by consumers during a compatibility window and MAY be deprecated by a separate future change. | ||
|
|
||
| The `0.2` build manifest SHALL replace the `assets` map with a single `files` map: canonical inner-tar path → `sha256:<hex>`, covering every entry — `facet.json`, primary asset files, and supplementary files. The map carries hashes only. Asset/supplementary classification is NEVER read from the build manifest; it is derived from the embedded `facet.json` via the archive plan (D3). The completeness rule is single: the `files` key set MUST exactly equal the observed inner-tar entry set. | ||
|
|
||
| Verifiers SHALL dispatch on `facetVersion` exactly once, at parse time: the exact legacy schema and rules apply to `0.1`; the rules above apply to `0.2`; any other version returns a structured `UNSUPPORTED_FACET_VERSION` failure carrying the observed version and supported versions. A malformed `0.2` manifest MUST NOT be reinterpreted as `0.1` — no fallback between versions. A `files` key in a `0.1` manifest (or an `assets` key in `0.2`) fails schema validation, making the illegal combinations unrepresentable in validated data. | ||
|
|
||
| The CLI SHALL render unsupported-version failures as upgrade guidance. For a known format transition, one CLI-side compatibility table SHALL map the facet format to the minimum supporting CLI release, producing guidance such as: “This facet uses archive format 0.2, which this CLI does not support. Update agent-facets to <minimum version> or later.” For an unknown future format, the CLI SHALL advise updating to the latest release without inventing a minimum version. Already-released CLIs cannot be retrofitted and MAY continue showing their existing generic validation error; a consumer-first bridge release SHOULD add this handling before any producer emits `0.2`. | ||
|
|
||
| This archive-format boundary is distinct from package release versioning. While `@agent-facets/protocol` and `@agent-facets/adapter` remain pre-1.0, breaking contract changes SHALL increment each package's minor version rather than its major version. This change therefore ships in the next minor release of each package. The permanent protocol release policy SHALL be updated by this change to encode the pre-1.0 rule; after 1.0, breaking changes SHALL require a major release. |
There was a problem hiding this comment.
D4 states that breaking changes ship in the next minor release while @agent-facets/protocol remains pre-1.0. However, openspec/specs/protocol/spec.md currently requires: "Backward-incompatible changes SHALL only be made in a new major version." The spec includes two concrete scenarios that apply directly here — narrowing a permitted value and removing a requirement — both of which mandate a major release.
The design says "The permanent protocol release policy SHALL be updated by this change to encode the pre-1.0 rule," but that update is deferred to the implementation phase (migration plan step 1). Until that spec update ships, implementers reading design.md and specs/protocol/spec.md get opposing instructions. D4 should cross-reference openspec/specs/protocol/spec.md §"Protocol requirements evolve under semantic-versioning discipline" and explicitly note that the minor-release policy becomes operative only after that spec section is amended.
| `facets.lock` SHALL use `lockfileVersion: 0.2` for the current alpha schema. Version dispatch MUST use exact equality, never numeric ordering: legacy numeric `1` identifies the previous alpha schema, while numeric `0.2` identifies this schema. `FACET_ARCHIVE_VERSION` and `LOCKFILE_VERSION` SHALL remain separate constants that both currently equal `0.2`; their equality is release alignment, not a permanent invariant, because archive and resolution formats may evolve independently. | ||
|
|
There was a problem hiding this comment.
0.2 as a lockfile version discriminant is ordering-inverted
D10 explicitly calls these "numeric" values: legacy 1 and current 0.2. Using a JSON float where 0.2 < 1 numerically — yet 0.2 is the newer schema — creates inverted ordering that will confuse implementers even with the correct exact-equality dispatch rule. String versions ("1" and "0.2") would be unambiguous across all JSON implementations, eliminate IEEE 754 concerns for 0.2, and work identically with exact-equality dispatch. If keeping numeric values, D10 should at minimum explain why the current schema is numerically smaller than the legacy one it supersedes.
| "authoredAt": "2026-07-13T18:10:26Z", | ||
| "comparedAt": "2026-07-13T18:19:40Z", | ||
| "reconciledAt": "2026-07-13T18:41:00Z", | ||
| "notes": "Adopted all 10 review findings (nothing material rejected; Main's structural base retained per the review itself). Added D3 shared tagged archive-plan derivation as single source of membership/classification; 0.2 build manifest replaces parallel assets+files maps with one all-entry hash map + strict version dispatch; protocol package and adapter SDK each marked as new major releases; D7 grammar expanded with exhaustive path/tar checks (NUL, drive prefixes, Unicode/case-fold aliases, prefix collisions, symlink/hardlink identity, duplicate headers, non-regular entries, dist/ cleanup ordering) + per-class test matrix; D8 adapter contract rewritten to tagged payload unions with atomic stage/commit/rollback skill bundles; D9 receipt rewritten to tagged records with complete owned path sets, legacy tuples refined to empty companion sets (removed the inconsistent one-install-cycle orphan claim), receipt+materialization as one rollback unit; D10 scaffold writes README undeclared during rollout window and edit gains scaffold-or-remove for vanished declarations; Migration Plan reordered consumer-first with cafe as hard gate and irreversibility note; old-builder-ignores-files hazard added to Risks; all three Open Questions closed as decisions." |
There was a problem hiding this comment.
The notes say "D10 scaffold writes README undeclared during rollout window," implying the create wizard should write README.md without adding it to files while consumers haven't yet deployed 0.2 support. D11 says the opposite: "the wizard SHALL atomically write README.md and add its exact path to top-level files" — unconditionally. The migration plan's sequencing (D11 behavior ships in step 5, after cafe deploys 0.2 support in step 3) already handles the rollout concern, making this note misleading to future readers auditing the trail.
1 participant
Why
The facet pipeline currently enforces a strict one-file-per-asset invariant throughout build, verification, installation, and the adapter contract. This makes it impossible to ship supplementary files — such as
README.md,LICENSE, or skill companion files — inside a facet archive. This change introduces explicit manifest-declared supplementary files, a newfacetVersion: 0.2archive format, and all the downstream contract changes required to keep membership, integrity, and materialization consistent across the entire pipeline.Details
The embedded
facet.jsonbecomes the sole source of truth for archive membership and entry classification. A single shared archive-plan derivation operation in the protocol package validates both declaration sites and produces a tagged plan (manifest,primary-asset,skill-companion,archive-only) that build collection, hashing, verification, parsed results, and installation all consume — no stage maintains its own membership logic.Declaration shape (D1/D2): Two disjoint declaration sites are introduced.
SkillDescriptor.filesdeclares companion paths relative to the skill directory; these are the only supplementary files that materialize, and they do so atomically with their owning skill. A top-levelfilesarray declares everything else (rootREADME.md,LICENSE, etc.) as archive-only metadata that ships and is integrity-protected but is never written to disk at install time. Globs and recursive discovery are explicitly excluded.Archive format (D4): All builds produced after this change emit
facetVersion: 0.2. The0.2build manifest replaces theassetshash map with a singlefilesmap covering every inner-archive entry. Classification is never read from the build manifest — it is always derived from the embedded manifest via the archive plan. Verifiers dispatch on version exactly once with no cross-version fallback;0.1archives remain valid during a compatibility window.Path and tar security (D5/D7): Archive verification validates raw tar headers before constructing any path-keyed map to prevent duplicate-header smuggling. The path grammar rejects empty/
./..segments, backslashes, NUL bytes, absolute and drive-prefixed paths, symlinks, hard links, Unicode/case-fold aliases, and file/directory prefix collisions. All source inputs are validated before anydist/cleanup runs. Every failure class maps to a distinct structured result variant, and the test suite requires at least one case per class.Adapter contract (D8): Install, read, and delete requests become tagged unions keyed by asset type. The skill variant carries
SKILL.mdtext plus a canonical companion-byte map; agent and command variants structurally cannot carry companions. A skill install is one all-or-nothing operation: stage the complete bundle, remove previously owned companions absent from the new bundle, and commit or roll back. Supplementary files outside skill directories never reach adapters.Lockfile and receipt (D10): Lockfile
0.2records the canonical path andsha256integrity of every materialized file inside its owning asset entry. Install reconciles these records against recomputed archive hashes before any writes and reports the exact mismatching path on failure. The machine-local receipt mirrors committed ownership for offline removal and rollback. Archive-only supplementary files are covered by facet-level integrity and do not appear as lockfile assets.Asset naming (D9): Current-format asset names follow the Agent Skills
namefield convention as single ASCII segments. Skills and commands share one logical namespace and must not use the same name. Legacy0.1archives retain their previous validation rules.README authoring (D11):
facet createscaffoldsREADME.mdby default.facet editexposes dedicated create/edit/adopt/scaffold/remove actions for bothREADME.mdand extensionlessREADME. Both remain ordinary top-level supplementary-file declarations.Rollout is consumer-first: verification support — including the cafe registry — must deploy before any producer emits
0.2. The protocol package and adapter SDK each ship these breaking changes in their next minor release while pre-1.0. The permanent release policy is updated to require major releases for breaking changes after 1.0.The design document (
design.md) and its adversarial review (adversarial/) are included alongside the updated proposal. The adversarial review identifies four blocking cross-cutting items — shared archive-plan derivation, atomic adapter/receipt lifecycle, strict version dispatch with consumer-first rollout, and a single hash-map source of truth — all of which are resolved in the reconciled design.Note
Low Risk
Documentation and design artifacts only; no runtime code changes. Future implementation will touch security-sensitive build, verification, install, and adapter boundaries described here.
Overview
Adds the adversarial design artifact and a Main vs. Adversary design review, and marks the design change reconciled in OpenSpec state with notes on what was merged.
design.mdis the reconciled technical design for manifest-declared supplementary files: dual declaration sites (SkillDescriptor.files+ top-levelfiles), a shared tagged archive-plan derivation, unconditionalfacetVersion: 0.2with a single all-entryfileshash map, strict verification (raw tar headers, path grammar, tagged parsed results), tagged adapter skill bundles with atomic install/delete, lockfile/receipt0.2with per-materialized-file integrity, Agent Skills–style single-segment naming, first-class README create/edit flows, and a consumer-first migration plan.proposal.mdis expanded to match: 0.2 asset naming and lockfile semantics, README authoring, explicit BREAKING archive/protocol boundaries, and updated capability and documentation impact (includinglockfile.mdx).Reviewed by Cursor Bugbot for commit d1621a4. Bugbot is set up for automated code reviews on this repo. Configure here.